GPC: Set extension based on header #3895

przemkaczmarek · 2024-08-31T12:12:05Z

Implements #2748

request_wrapper.go 2748

exchange/exchange.go

openrtb_ext/request_wrapper.go

endpoints/openrtb2/auction.go

endpoints/openrtb2/auction_test.go

bsardo · 2024-09-05T14:07:17Z

endpoints/openrtb2/auction.go

-	dntEnabled  int8   = 1
-	notAmp      int8   = 0
+	dntKey       string = http.CanonicalHeaderKey("DNT")
+	secGPCHdrKey string = http.CanonicalHeaderKey("Sec-GPC")


Nitpick: secGPCKey suffices

bsardo · 2024-09-05T14:12:04Z

endpoints/openrtb2/auction_test.go

-		expectError bool
+		description  string
+		headerValue  string
+		expectedGPC  string


You can remove this because expectedRegs should contain the expected GPC value.

bsardo · 2024-09-10T16:49:47Z

endpoints/openrtb2/auction.go

@@ -1516,6 +1522,25 @@ func setAuctionTypeImplicitly(r *openrtb_ext.RequestWrapper) {
 	}
 }

+func setGPCImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) error {
+	secGPC := httpReq.Header.Get(secGPCHdrKey)
+	fmt.Printf("Sec-GPC Header: %s\n", secGPC)


Remove print statement

bsardo · 2024-09-10T16:50:57Z

endpoints/openrtb2/auction_test.go

+		header       string
+		expectedGPC  string
+		expectError  bool
+		regs         *openrtb2.Regs


Nitpick: move regs up a line so all of the expected fields are at the end of the struct definition. Please also do this in the actual test cases.

bsardo · 2024-09-10T16:54:16Z

openrtb_ext/request_wrapper.go

+	GPC := *re.gpc
+	return &GPC


Nitpick: GPC variable name should be gpc in accordance with golang best practices

bsardo · 2024-09-10T16:55:03Z

openrtb_ext/request_wrapper.go

The code looks good. Please update openrtb_ext/request_wrapper_test.go with test coverage for all of your changes in this file.

Not sure if you missed this during your last iteration but please add/update the wrapper tests to cover changes.

bsardo · 2024-09-10T17:02:08Z

endpoints/openrtb2/auction.go

+
+	gpc := "1"
+	regExt.SetGPC(&gpc)


I suggest adding the following early return just before the set:

if regExt.GetGPC() != nil { return nil }

This way we won't perform an unnecessary write which sets the dirty flag and causes extra work when rebuilding the request downstream.

Hi @przemkaczmarek, here is a super nitpick to this function.
In the issue description the logic is straightforward:

1. If the incoming request already contains regs.ext.gpc, use that. 2. Otherwise, if there's an HTTP header for Sec-GPC with a value of "1" or 1, set regs.ext.gpc to "1".

The code here does the right thing, however it's a little confusing and difficult to read. I had to read it couple of times to understand it.
Here is a suggestion that I think traces better to the requirements and might be better for readability:

func setGPCImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) error { regExt, err := r.GetRegExt() if err != nil { return err } if regExt.GetGPC() != nil { return nil } secGPC := httpReq.Header.Get(secGPCKey) if secGPC == "1" { gpc := "1" regExt.SetGPC(&gpc) } return nil }

The unit test passes as is.

What do you think?

In my version of the code, I first check the value of secGPC, and if it doesn't meet the condition, the function ends immediately with return nil. In your version of the code, this happens later, which leads to unnecessary retrieval of regExt, even when it's not needed. I avoid unnecessary operations. I avoid performing costly operations (such as r.GetRegExt()) if the secGPC header does not have the value '1'. In the first version, you retrieve the regExt extension regardless of the secGPC value, which is less optimal

Yes, I understand this. I am only concerned about the readability.
Also this is a minor performance optimization (but still an optimization!) so it's up to you to change it.
I approve this PR, and if you decide to change it - I'll re-approve it right away too.

I prefer to keep the code as is but thanks for the comment

bsardo · 2024-09-10T17:29:34Z

endpoints/openrtb2/auction_test.go

+			},
+		},
+		{
+			description: "sec_gpc_header_not_set_gpc_should_not_be_modified",


I suggest covering the following test cases:
regs_ext_gpc_not_set_and_header_not_set
regs_ext_gpc_not_set_and_header_is_1
regs_ext_gpc_not_set_and_header_not_1
regs_ext_gpc_is_1_and_header_is_1
regs_ext_gpc_is_1_and_header_not_1
regs_ext_other_data_and_header_is_1
regs_ext_nil_and_header_is_1
regs_nil_and_header_is_1

bsardo · 2024-09-13T18:11:38Z

endpoints/openrtb2/auction_test.go

+			expectedRegs: &openrtb2.Regs{
+				Ext: []byte(`{"gpc":"1"}`),
+			},
+		},


I think we need to add these two test cases to ensure that we don't create a regs or regs.ext object if we're not writing to the gpc field:

{ description: "regs_nil_and_header_not_set", header: "", regs: nil, expectError: false, expectedRegs: nil, }, { description: "regs_ext_is_nil_and_header_not_set", header: "", regs: &openrtb2.Regs{ Ext: nil, }, expectError: false, expectedRegs: &openrtb2.Regs{ Ext: nil, }, },

bsardo · 2024-09-13T18:12:25Z

endpoints/openrtb2/auction_test.go

+				assert.NoError(t, err)
+			}
+			assert.NoError(t, r.RebuildRequest())
+			assert.JSONEq(t, string(test.expectedRegs.Ext), string(r.BidRequest.Regs.Ext))


In order to handle the two new test cases I requested you can modify this as such:

if test.expectedRegs == nil { assert.Nil(t, r.BidRequest.Regs) } else if test.expectedRegs.Ext == nil { assert.Nil(t, r.BidRequest.Regs.Ext) } else { assert.JSONEq(t, string(test.expectedRegs.Ext), string(r.BidRequest.Regs.Ext)) }

bsardo · 2024-09-13T18:13:32Z

openrtb_ext/request_wrapper.go

Not sure if you missed this during your last iteration but please add/update the wrapper tests to cover changes.

bsardo · 2024-09-16T15:53:21Z

openrtb_ext/request_wrapper_test.go

@@ -2253,6 +2254,21 @@ func TestRegExtUnmarshal(t *testing.T) {
 			expectGDPR:  ptrutil.ToPtr[int8](0),
 			expectError: true,
 		},
+		// GPC


These tests look good as they adequately cover your changes to the RegExt unmarshal function. However, there are still other request wrapper changes that require test coverage:

Update TestRebuildRegExt, which will cover your RegExt marshal changes

Add a new test TestRegExtGetGPCSetGPC that is similar to TestRegExtGetGDPRSetGDPR

przemkaczmarek · 2024-09-18T14:04:30Z

@bsardo Should I accept ? or Do You add another reviewer?

bsardo · 2024-09-18T16:31:16Z

@bsardo Should I accept ? or Do You add another reviewer?

We need two approvals to merge. I'll see if @VeronikaSolovei9 can look at it today.

@dkornet-ad

commit df58baf Author: Brian Sardo <[email protected]> Date: Tue Oct 29 21:05:07 2024 -0400 Cookie Sync: Use max when limit is 0 (prebid#4022) commit db2a872 Author: Brian Sardo <[email protected]> Date: Tue Oct 29 17:20:57 2024 -0400 ORTB 2.6: Full support with down convert for 2.5 adapters (prebid#4019) Co-authored-by: hhhjort <[email protected]> Co-authored-by: Veronika Solovei <[email protected]> commit 1419761 Author: Scott Kay <[email protected]> Date: Tue Oct 29 16:35:05 2024 -0400 Rename Blacklisted Apps to Blocked Apps (prebid#3620) commit ddf897c Author: AlexBVolcy <[email protected]> Date: Tue Oct 29 14:07:49 2024 -0400 Update Cookie Sync Chooser Debug Message to match Java (prebid#3558) commit a23380f Author: Mohammad Nurul Islam Shihan <[email protected]> Date: Tue Oct 29 21:34:31 2024 +0600 ImproveDigital: Remove placementKey & addtlconsent parsing (prebid#3728) commit 03a4abd Author: Alex Maltsev <[email protected]> Date: Tue Oct 22 22:26:45 2024 +0300 Sample: Fix prebid js loading bug on sample html page (prebid#3792) commit 18f6798 Author: Sebastien Boisvert <[email protected]> Date: Tue Oct 22 15:15:07 2024 -0400 Bump Go version to 1.22 in dev containers config (prebid#3983) commit bcf6491 Author: sindhuja-sridharan <[email protected]> Date: Thu Oct 17 15:20:45 2024 -0600 GumGum: Declare ORTB 2.6 support (prebid#3985) commit 451bc44 Author: Bluesea <[email protected]> Date: Thu Oct 17 03:12:50 2024 +0800 BlueSea: Add site capability (prebid#3910) Co-authored-by: prebid-bluesea <[email protected]> commit 8134328 Author: Boris Yu <[email protected]> Date: Wed Oct 16 21:08:50 2024 +0300 Displayio: Make imp.bidfloor optional (prebid#3959) commit b56923c Author: Piotr Jaworski <[email protected]> Date: Tue Oct 15 21:49:39 2024 +0200 RTB House: Resolve AUCTION_PRICE macro (prebid#3901) commit cbe9876 Author: Irakli Gotsiridze <[email protected]> Date: Tue Oct 15 23:30:17 2024 +0400 enable gzip (prebid#3946) commit 5fcbbbf Author: Patrick Loughrey <[email protected]> Date: Tue Oct 15 15:19:47 2024 -0400 Triplelift: Adding flag for 2.6 support (prebid#3966) commit c37951a Author: ym-winston <[email protected]> Date: Tue Oct 15 15:18:54 2024 -0400 update yieldmo.yaml to indicate support for ortb 2.6 (prebid#3968) commit 9bb9b3d Author: bhainesnexxen <[email protected]> Date: Tue Oct 15 12:09:33 2024 -0700 Unruly: Indicate Support for OpenRTB 2.6 (prebid#3984) commit 87d4412 Author: Brian Sardo <[email protected]> Date: Wed Oct 9 19:08:14 2024 +0000 Refactor: Move privacy init up (prebid#3958) commit 64584f6 Author: Brian Sardo <[email protected]> Date: Tue Oct 8 19:52:25 2024 +0000 Refactor: Remove getAuctionBidderRequests (prebid#3957) commit d54c3ed Author: ysfbsf <[email protected]> Date: Tue Oct 8 15:42:40 2024 +0200 New Adapter: Missena (prebid#3761) commit 8689e0d Author: Nick <[email protected]> Date: Mon Oct 7 10:39:59 2024 -0400 Sonobi: native and currency conversion support (prebid#3889) Co-authored-by: bansawbanchee <[email protected]> commit f27bcef Author: dkornet-ad <[email protected]> Date: Thu Oct 3 09:33:54 2024 +0300 New Adapter: Bidmatic (prebid#3731) authored by @dkornet-ad commit 6c154e0 Author: Alexander Pykhteyev <[email protected]> Date: Wed Sep 25 23:08:23 2024 +0700 New Adapter: Streamlyn (prebid#3900) Co-authored-by: apykhteyev <[email protected]> commit 53f51a6 Author: przemkaczmarek <[email protected]> Date: Wed Sep 25 17:53:07 2024 +0200 GPC: Set extension based on header (prebid#3895) commit c42fe53 Author: bkaneyama <[email protected]> Date: Wed Sep 25 07:46:01 2024 -0700 InMobi: mtype support (prebid#3921) commit 8b1b96e Author: Brian Sardo <[email protected]> Date: Wed Sep 25 14:22:18 2024 +0000 Add docs build readme (prebid#3930) commit 4462fce Author: Eugene Dorfman <[email protected]> Date: Wed Sep 25 16:11:45 2024 +0200 51degrees module (prebid#3893) commit 11b6546 Author: Alex Maltsev <[email protected]> Date: Mon Sep 23 16:54:18 2024 +0300 Rubicon: Pass PBS host info to XAPI (prebid#3903) commit 93368cc Author: rtuschkany <[email protected]> Date: Thu Sep 19 23:45:32 2024 +0200 ConnectAd: String/int fix (prebid#3925) commit 905b3a5 Author: ShriprasadM <[email protected]> Date: Thu Sep 19 18:30:20 2024 +0530 Log non bid reasons in bidder framework (prebid#2891) Co-authored-by: Shriprasad Marathe <[email protected]> Co-authored-by: ashish.shinde <[email protected]> Co-authored-by: dhruv.sonone <[email protected]> commit 3c4527e Author: Mikael Lundin <[email protected]> Date: Wed Sep 18 13:38:49 2024 +0200 Adnuntius: Return DSA in bid response (prebid#3777) commit 640b97c Author: lm-ved <[email protected]> Date: Mon Sep 16 12:17:54 2024 +0530 LemmaDigital: change endpoint (prebid#3862) commit 3e24be7 Author: Alexander Pykhteyev <[email protected]> Date: Sat Sep 14 04:04:09 2024 +0700 New Adapter: TGM (prebid#3848) Co-authored-by: apykhteyev <[email protected]> commit ffdd75f Author: Robert Kawecki <[email protected]> Date: Thu Sep 12 21:52:51 2024 +0200 New Adapter: AdTonos (prebid#3853) commit c02ee8c Author: Laurentiu Badea <[email protected]> Date: Thu Sep 12 12:34:46 2024 -0700 Update exchange json tests with correct hb_pb_cat_dur (prebid#3836) commit 7613ff5 Author: Laurentiu Badea <[email protected]> Date: Thu Sep 12 12:27:07 2024 -0700 Update adapter json test framework to validate BidVideo (prebid#3835) commit 6cbedf0 Author: schubert-sc <[email protected]> Date: Wed Sep 11 19:41:53 2024 +0300 Smartx: Declare OpenRTB 2.6 support (prebid#3896) commit e0a21d0 Author: qt-io <[email protected]> Date: Wed Sep 11 19:31:58 2024 +0300 QT: Add tcfeu support (prebid#3892) Co-authored-by: qt-io <[email protected]> commit b920cca Author: Yanivplaydigo <[email protected]> Date: Wed Sep 11 19:19:12 2024 +0300 Playdigo: Add tcfeu support (prebid#3890) commit 6a011ed Author: ccorbo <[email protected]> Date: Wed Sep 11 09:53:54 2024 -0400 Update github.com/rs/cors to v1.11.0 (prebid#3884) Co-authored-by: Chris Corbo <[email protected]> commit ec6a45d Author: Steffen Müller <[email protected]> Date: Wed Sep 4 15:51:44 2024 +0200 Agma: Allow app.bundle to be used as selector for apps (prebid#3780) commit 8237f7f Author: Scott Kay <[email protected]> Date: Tue Sep 3 14:34:56 2024 -0400 Refactor Bid Splitter Privacy Functions (prebid#3645) commit f7caea5 Author: Brian Schmidt <[email protected]> Date: Fri Aug 30 06:47:45 2024 -0700 OpenX: indicate support for OpenRTB 2.6 (prebid#3879) commit e825553 Author: Ben Oraki <[email protected]> Date: Fri Aug 30 16:36:22 2024 +0300 New Adapter: Oraki (prebid#3839) commit 2e2b49f Author: escalax <[email protected]> Date: Fri Aug 30 16:28:04 2024 +0300 New Adapter: Escalax (prebid#3798) commit 4ea0e33 Author: Copper6SSP <[email protected]> Date: Fri Aug 30 16:18:46 2024 +0300 New Adapter: Copper6SSP (prebid#3755) commit 8d7117d Author: Brian Sardo <[email protected]> Date: Tue Aug 27 13:23:34 2024 -0400 Revert "New Module: 51Degrees (prebid#3650)" (prebid#3888) This reverts commit 2606e75. commit 84a8162 Author: Anand Venkatraman <[email protected]> Date: Thu Aug 22 16:14:32 2024 +0530 PulsePoint: ortb 2.6 version and gpp support (prebid#3874) authored by @anand-venkatraman commit bd85ba4 Author: Nick Llerandi <[email protected]> Date: Thu Aug 22 02:15:12 2024 -0400 specifies ortb 2.6 support (prebid#3) (prebid#3876) commit 54f8759 Author: dtbarne <[email protected]> Date: Wed Aug 21 07:26:30 2024 -0500 Update mobilefuse.yaml to indicate support for OpenRTB 2.6 and GPP (prebid#3871) commit 59a5b07 Author: mwang-sticky <[email protected]> Date: Wed Aug 21 20:26:15 2024 +0800 freewheel-adapter: support 2.6 (prebid#3873) commit a556e2d Author: Dubyk Danylo <[email protected]> Date: Wed Aug 21 15:23:39 2024 +0300 Fix currency conversion bug. (prebid#3867) Co-authored-by: ddubyk <[email protected]> commit d161712 Author: Pubrise <[email protected]> Date: Wed Aug 21 12:43:39 2024 +0300 new adapter (prebid#3833) authored by @Pubrise commit e8509e6 Author: bretg <[email protected]> Date: Wed Aug 21 05:40:26 2024 -0400 declare support for ORTB 2.6 (prebid#3872) authored by @bretg commit 6be7244 Author: Saar Amrani <[email protected]> Date: Wed Aug 21 12:37:59 2024 +0300 Update Vidazoo bidder info for GPP support (prebid#3869) commit e4bd6d3 Author: gg-natalia <[email protected]> Date: Sun Aug 18 06:52:28 2024 -0300 ADTS-455 remove video validations (prebid#3842) authored by @gg-natalia commit 2606e75 Author: James Rosewell <[email protected]> Date: Fri Aug 16 14:20:22 2024 +0100 New Module: 51Degrees (prebid#3650) Co-authored-by: James Rosewell <[email protected]> Co-authored-by: Marin Miletic <[email protected]> Co-authored-by: Sarana-Anna <[email protected]> Co-authored-by: Eugene Dorfman <[email protected]> Co-authored-by: Krasilchuk Yaroslav <[email protected]> commit 0e9b234 Author: Antonios Sarhanis <[email protected]> Date: Tue Aug 13 18:28:44 2024 +1000 Use format=prebid on adserver requests. (prebid#3846) commit 4d64623 Author: ownAdx <[email protected]> Date: Mon Aug 12 18:51:53 2024 +0530 OwnAdx: Bidder param and URL updates (prebid#3813) Co-authored-by: Hina Yadav <[email protected]> commit 2a19924 Author: ahmadlob <[email protected]> Date: Mon Aug 12 16:09:23 2024 +0300 taboola-support-app-in-prebid-server (prebid#3795) commit 4f177ca Author: benben2001 <[email protected]> Date: Mon Aug 12 21:07:38 2024 +0800 New Adapter: MeloZen (prebid#3784)

This reverts commit 96cbdb3.

Add GPC extension handling to exchange.go and

8971a36

request_wrapper.go 2748

przemkaczmarek requested a review from bsardo August 31, 2024 12:12

bsardo changed the title ~~Add GPC extension handling to exchange.go and~~ GPC: Set extension based on header Sep 3, 2024

bsardo self-assigned this Sep 3, 2024

bsardo reviewed Sep 4, 2024

View reviewed changes

exchange/exchange.go Outdated Show resolved Hide resolved

exchange/exchange.go Outdated Show resolved Hide resolved

exchange/exchange.go Outdated Show resolved Hide resolved

openrtb_ext/request_wrapper.go Outdated Show resolved Hide resolved

bsardo added the do not port label Sep 4, 2024

przemkaczmarek added 2 commits September 4, 2024 14:02

example

58aab7c

example

b7692d7

bsardo reviewed Sep 4, 2024

View reviewed changes

przemkaczmarek added 2 commits September 5, 2024 08:58

fix bugs

a2b7cf1

fix comments

c7fbfe1

bsardo reviewed Sep 10, 2024

View reviewed changes

przemkaczmarek added 3 commits September 11, 2024 14:48

fix comments

f713ff1

fix regs_nil_and_header_not_set test

5fba3db

checking validation

f8e7977

bsardo reviewed Sep 13, 2024

View reviewed changes

fix comments

b5acce3

bsardo reviewed Sep 16, 2024

View reviewed changes

additional test in request_wrapper_test

bff49e2

bsardo approved these changes Sep 17, 2024

View reviewed changes

bsardo assigned VeronikaSolovei9 Sep 18, 2024

VeronikaSolovei9 approved these changes Sep 20, 2024

View reviewed changes

bsardo merged commit 53f51a6 into master Sep 25, 2024
3 checks passed

krdzo pushed a commit to Ogury/prebid-server that referenced this pull request Oct 30, 2024

GPC: Set extension based on header (prebid#3895)

96cbdb3

krdzo added a commit to Ogury/prebid-server that referenced this pull request Oct 30, 2024

Revert "GPC: Set extension based on header (prebid#3895)"

4d0d8e0

This reverts commit 96cbdb3.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPC: Set extension based on header #3895

GPC: Set extension based on header #3895

przemkaczmarek commented Aug 31, 2024 •

edited by bsardo

Loading

bsardo Sep 5, 2024

bsardo Sep 5, 2024

bsardo Sep 10, 2024

bsardo Sep 10, 2024

bsardo Sep 10, 2024

bsardo Sep 10, 2024

bsardo Sep 13, 2024

bsardo Sep 10, 2024

VeronikaSolovei9 Sep 19, 2024

przemkaczmarek Sep 20, 2024

VeronikaSolovei9 Sep 20, 2024

przemkaczmarek Sep 25, 2024

bsardo Sep 10, 2024

bsardo Sep 13, 2024 •

edited

Loading

bsardo Sep 13, 2024

bsardo Sep 13, 2024

bsardo Sep 16, 2024

przemkaczmarek commented Sep 18, 2024

bsardo commented Sep 18, 2024

GPC: Set extension based on header #3895

GPC: Set extension based on header #3895

Conversation

przemkaczmarek commented Aug 31, 2024 • edited by bsardo Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemkaczmarek commented Sep 18, 2024

bsardo commented Sep 18, 2024

przemkaczmarek commented Aug 31, 2024 •

edited by bsardo

Loading

bsardo Sep 13, 2024 •

edited

Loading